[VC-59470] Start only the matching e2e suite when its label is added - #837
Conversation
wallrj-cyberark
added
the
kind/cleanup
Verified: the label alone started a runAdding a label to this pull request started a second workflow run, with no push. That is the behaviour that does not happen on
The two runs share the same head SHA, so no commit was pushed between them. Run 2 was created three seconds after I added I used What this run does not proveIt shows the If you would rather see it end to end before merging, say so and I will add |
c83c994 to
cb858f9
Compare
wallrj-cyberark
changed the title
wallrj-cyberark
added
kind/cleanup
Verified on this pull requestRevised after review feedback: an unrelated label used to re-run
Run 1 is the no-regression check: ordinary pushes still behave exactly as before. Run 2 is the fix you asked for. The direct before/after is on the same pull request and the same action — adding Run 3 is the positive half, and the only real proof that I cancelled run 3 about 45 seconds in, once the job selection was visible, to avoid a full 30-minute QA run nobody asked for. It had reached the first seconds of One behaviour change to know aboutAdd Not covered by these runsNo GKE cluster was created at any point, so the |
wallrj-cyberark
changed the title
ReviewThe trigger fix itself is correct. I walked every event this workflow subscribes to through all five gates and found no hole: One notable finding, and one minor note. 1. A label event replaces the
|
| Run | Trigger | verify |
test |
|---|---|---|---|
| 34487851234 14:15Z | synchronize | success | success |
| 34488035695 14:17Z | labeled | skipped | skipped |
| 34488147664 14:18Z | labeled | skipped | skipped |
gh pr checks 837 currently reports verify skipping and test skipping. The unit tests did pass on this commit, but nothing in the checks list says so any more.
Merging should not be blocked by this — GitHub counts a job-level if: skip as satisfying a required status check. The cost is the signal, not the gate: "skipped because somebody added a label" is now indistinguishable from "never ran", both to a reviewer glancing at the checks list and to anything automated that reads conclusions off the head SHA. That is a bit more than the "line in the Actions list" the description prices it at, and it lands on every pull request in the repo, including ones where the label had nothing to do with e2e.
The clean fix is to not subscribe tests.yaml to labeled at all: move the three e2e jobs into their own workflow file taking [opened, synchronize, reopened, labeled], and leave tests.yaml on the default activity types. A label event then creates check runs only for the e2e jobs and never touches verify or test, which also makes the two if: github.event.action != 'labeled' guards unnecessary. That is a larger change than this one, so whether it belongs here or in a follow-up is your call — as it stands this is still a clear improvement on a label doing nothing at all.
2. Minor: the keep-e2e-cluster ordering rule only exists in this description
Adding test-e2e now starts the GKE run immediately, so the window to add keep-e2e-cluster first is gone the moment you apply the label — and getting the order wrong silently deletes the cluster you wanted to keep. Worth putting that sentence in a comment next to the Delete GKE Cluster step, where somebody debugging will actually encounter it, rather than leaving it in a merged pull request description.
[Generated with claude code]
Adding test-e2e, test-ark or test-ngts to an open pull request currently
does nothing. The workflow uses `on: pull_request: {}`, which takes the
default activity types of opened, synchronize and reopened. There is no
`labeled`, so the label sits on the pull request and no run starts.
Re-running does not help either, because a re-run replays the original
event payload, which had no labels. The only way through is to close and
reopen the pull request, or push a commit. The failure mode is silent,
so it looks like the job is broken.
Move the three e2e jobs into a new e2e.yaml, which subscribes to
`labeled` as well as the default types. Each one runs only when
github.event.label.name, the single label that was just added, is its
own label, so adding test-ngts does not start the ark or GKE suites.
Keeping them in tests.yaml would have cost the verify and test results.
A skipped job still publishes a check run, and GitHub shows the most
recent one per job name, so a label event would have replaced their
green results with "skipped". Splitting the file means a label event
never creates a check run for verify or test at all.
tests.yaml therefore keeps the default activity types and needs no
job-level guards.
Behaviour on push, on workflow_dispatch and on opened, synchronize and
reopened is unchanged, so a suite whose label is already on the pull
request still re-runs on every new commit. The e2e jobs never ran on
push, so dropping that trigger from e2e.yaml changes nothing.
Also document that keep-e2e-cluster must be added before test-e2e. The
cleanup step reads the labels from the event payload, frozen when the
run started, and test-e2e now starts the run immediately.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Richard Wall <richard.wall@cyberark.com>
cb858f9 to
0b4c90c
Compare
wallrj-cyberark
added
kind/cleanup
|
@FelixPhipps thank you, finding 1 was right and I have taken your suggested fix. Both points are addressed in 0b4c90c. 1. Check runs — fixed by splitting the workflowI confirmed your finding before acting on it. I have done what you suggested rather than leaving it as a follow-up. The three e2e jobs now live in Verified live on 0b4c90c:
Step 2 is the direct before/after against the state you documented. One residual I should name rather than let you find: a label event still republishes the two non-matching e2e jobs as I cancelled run 3 once the job selection was visible, to avoid a 30 minute QA run, then re-ran 35341615700 so the checks list is not left showing a cancelled job. 2.
|
|
Both points addressed — thanks for taking the split rather than deferring it. Agreed on the residual: republishing non-matching e2e jobs as LGTM! |
2 participants
Adding the
test-e2e,test-arkortest-ngtslabel to an open pull request does not start a run. This makes it start one — and only the suite you asked for.Why now?
I hit this on #835. I added the
test-e2elabel, nothing happened, and I had to force-push to fire asynchronizebefore the e2e would start. It looks exactly like a broken job.The cause is that
tests.yamluseson: pull_request: {}, which takes the default activity types.— Events that trigger workflows
labeledis not in that list, so the label sits there and no run starts. Re-running does not help either, because a re-run replays the original event payload, which had no labels.So the label route documented in #833 has always needed a push afterwards. #833 gave us a reliable manual route with
workflow_dispatch, which covers the release case. This fixes the pull request case.What changes
The three e2e jobs move into a new
.github/workflows/e2e.yaml, which subscribes tolabeledas well as the default activity types. Each one runs only ifgithub.event.label.name— the single label that was just added — is its own label. Addingtest-ngtsdoes not start the ark or GKE suites.tests.yamlkeepsverifyandtest, keeps the default activity types, and needs no job-level guards.Why a separate file, rather than adding
labeledtotests.yaml? Because a skipped job still publishes a check run, and GitHub shows the most recent one per job name. A label event in a workflow containingverifyandtestreplaces their green results withskipped. Splitting means a label event never creates a check run for them at all. Thanks to @FelixPhipps for catching this — an earlier revision of this pull request had exactly that bug, and you can see it in the review thread below.Nothing else changes. Push,
workflow_dispatchandopened/synchronize/reopenedbehave as they do today, so a suite whose label is already on the pull request still re-runs on every new commit. The e2e jobs never ran on push, so dropping that trigger frome2e.yamlchanges no behaviour.Action required when you use
keep-e2e-clusterAdd
keep-e2e-clusterbeforetest-e2e, not after. The cleanup step reads the labels from the event payload, frozen when the run started, andtest-e2enow starts the run immediately. Get the order wrong and the cluster you wanted to keep is deleted. This is now written next to theDelete GKE Clusterstep, inCONTRIBUTING.mdand inREADME.md.Full behaviour table — what each event runs, and what it publishes as skipped
skippedverify,testworkflow_dispatchontestsverify,testworkflow_dispatchone2everify,testtest-e2everify,test,test-e2eark-test-e2e,ngts-test-e2ekind/cleanupaddedtest-e2eaddedtest-e2eark-test-e2e,ngts-test-e2etest-ngtsadded,test-e2ealready onngts-test-e2eark-test-e2e,test-e2ekeep-e2e-clusteraddedverifyandtestnever appear in the right-hand column. That is the property the split buys.There is a residual in the last three rows: a label event still republishes the two non-matching e2e jobs as
skipped, so it can overwrite an earlier greentest-e2e. Eliminating that entirely would need one workflow file per suite, which is three copies of the same boilerplate. Sinceskippedis the normal resting state for an opt-in e2e job, that did not seem worth it. Say if you disagree.Verified live: a label no longer touches the verify and test results
Evidence is in the comments below. The short version, all on commit 0b4c90c:
tests(verify,testboth green) ande2e(all three skipped).kind/cleanupcreated ane2erun only. Notestsrun was created at all, andgh pr checksstill reportedverify passandtest pass.test-ngtsstartedngts-test-e2ealone, withark-test-e2eandtest-e2eskipped and still notestsrun.What this does not fix
master, on merge or on a schedule. A greenmastertherefore still does not mean the e2e suites passed. The split makes adding aschedule:trigger easier, since there is now a workflow that contains only the e2e jobs.ark-test-e2estill carries itsTEMPORARYcomment about a recurring 400 "conflicting tagging values" error. Whether that flake is still real is unanswered, and it is the thing blocking a nightly run.Both are tracked separately.
[with Claude]